Implement #9650 Add parameter hooks to inventory plugin iocage by vbotka · Pull Request #9651 · ansible-collections/community.general

vbotka · 2025-01-30T13:11:07Z

SUMMARY

Implement #9650 Add parameter hooks to inventory plugin iocage.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

iocage.py

ADDITIONAL INFORMATION

Tested in Ubuntu 24.04

felixfontein

Thanks for your contribution!

I'm wondering a bit about the term hook. Is this some nomenclature that's used in the iocage surroundings for such things? If not, maybe another name would be better. I'm not very good at coming up with names, but maybe something like read_file_contents, resulting in iocage_file_contents being set?

felixfontein · 2025-01-31T21:27:33Z

+                        p = Popen(cmd_cat_hook, stdout=PIPE, stderr=PIPE, env=my_env)
+                        stdout, stderr = p.communicate()
+                        if p.returncode != 0:
+                            iocage_hooks.append('-')


Maybe a warning should be printed in this case? (Or the behavior should be configurable - ignore, warn, error.)

The jails may be heterogeneous, and a hook that works for one jail may not work for the other. I want to keep the spirit of silently ignoring No such file or any other error:

I don't want to complicate the use case where different jails use different hooks or no hooks at all. List all hooks and let the compose option pick what is needed.

The admins should be responsible for intercepting anything. And they should be used to it, especially in the case of the hooks. For example, the /etc/dhclient-enter-hooks and /etc/dhclient-exit-hooks silently ignore any failing lines in the scripts. It is expected, that the admin is responsible for checking what a hook is doing. There are also security implications.

We can add the options (ignore, warn, error) later.

felixfontein · 2025-01-31T21:28:09Z

+                            raise AnsibleError(f'Invalid (non unicode) input returned: {e}') from e
+
+                    except Exception:
+                        iocage_hooks.append('-')


Same here.

Also, why not using None instead of -? None can never appear as a real value, so you can distinguish "error happened" from "- was actually read".

The dash "-" is used in iocage to represent a missing value. See for example ioc_list.py#L259 or ioc_list.py#L276. We've already used it too:

if iocage_ip4_dict['ip4']: iocage_ip4 = ','.join([d['ip'] for d in iocage_ip4_dict['ip4']]) else: iocage_ip4 = '-'

vbotka · 2025-01-31T22:14:49Z

I'm wondering a bit about the term hook. Is this some nomenclature that's used in the iocage surroundings for such things? If not, maybe another name would be better.

The nomenclature is general. A hook is a method to intercept . For example,

In particular, dhclient-script says:

... dhclient-script will check for the existence of /etc/dhclient-enter-hooks. If found, it will be sourced (see sh(1)). After taking action according to $reason, dhclient-script will check for the existence of /etc/dhclient-exit-hooks. If found, it will be sourced (see sh(1)). These hooks scripts can be used to dynamically modify the environment at appropriate times during the DHCP negotiations.

There is no better name.

Co-authored-by: Felix Fontein <felix@fontein.de>

felixfontein · 2025-02-01T08:05:37Z

But generally hooks are running code. I would expect to be able to provide a script that runs something when reading "hook". So I think the name "hook" is not a good choice in this context.

vbotka · 2025-02-01T11:08:41Z

I see. I renamed the parameter to hooks_results.

russoz

LGTM as far as I can tell.

One thing I noticed, but probably for a different PR, is that the YAML indentation is not consistent within the file (or with other plugins for that matter).

vbotka · 2025-02-04T00:03:58Z

... YAML indentation is not consistent within the file ...

Thank you for the review!

I fixed the 4-space indentation in the DOCUMENTATION block. There is the 2-space indentation in the EXAMPLES block. Both blocks pass the YAML lint. YAML doesn't enforce the indentation size. The syntax of the YAML blocks is correct.
I assume the 4 spaces in DOCUMENTATION might be needed to merge the fragments, but I'm not sure when I see the inconsistencies for example in constructed.py. The rendering is horrible

shell> ansible-doc -t inventory community.general.iocage
  ...
   keyed_groups  Add hosts to group based on the values of a variable.
        default: []
        elements: dict
        type: list
        suboptions:

           default_value          The default value when the host variable's value is an empty
                          string.
                          This option is mutually exclusive with
                          `keyed_groups[].trailing_separator'.
          default: null
          type: str
          added in: version 2.12 of ansible-core

           key          The key from input dictionary used to generate groups.
          default: null
  ...

felixfontein · 2025-02-08T21:00:24Z

I fixed the 4-space indentation in the DOCUMENTATION block. There is the 2-space indentation in the EXAMPLES block. Both blocks pass the YAML lint. YAML doesn't enforce the indentation size. The syntax of the YAML blocks is correct.

The 2-space indentation is a kind of new requirement of community.general that's not yet checked by CI. (Also because ansible-test is trying to make collection maintainer's life difficult by not adding any features that are useful only for collections.)

The indentation in the YAML has no influence on the rendering in the ansible-doc text output. This looks like a bug in ansible-doc which applies spacing the wrong way in suboptions for the first line of the first paragraph. I'm also getting the same broken output in ansible-doc -t inventory ansible.builtin.constructed. (There are also other problems there, like line wrapping sometimes being broken for the first line of a paragraph, also for top-level options.)

vbotka · 2025-02-09T01:02:13Z

The 2-space indentation is a kind of new requirement of community.general

I fixed the 2-space indentation of the DOCUMENTATION block. Thank you.

felixfontein · 2025-02-09T12:31:24Z

I created a PR to fix the formatting problem in ansible-doc: ansible/ansible#84690

Co-authored-by: Felix Fontein <felix@fontein.de>

vbotka · 2025-02-11T14:43:04Z

To avoid potential complaints I added a note about the pool mountpoint. Sorry for the late commit.

patchback · 2025-02-11T17:05:31Z

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/fdd1331e4a333499cb21886f5f68dcbd3d4fa32a/pr-9651

Backported as #9731

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* Add parameter hooks to inventory plugin iocage. * Add changelog fragment. * Update plugins/inventory/iocage.py Co-authored-by: Felix Fontein <felix@fontein.de> * Parameter renamed to hooks_results * Fix DOCUMENTATION YAML 4-space indentation. * Fix DOCUMENTATION YAML 2-space indentation. * Update changelogs/fragments/9651-iocage-inventory-hooks.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Add note about activated pool mountpoint. --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit fdd1331)

felixfontein · 2025-02-11T17:05:47Z

@vbotka thanks for your contribution!
@russoz thanks for reviewing!

… hooks to inventory plugin iocage (#9731) Implement #9650 Add parameter hooks to inventory plugin iocage (#9651) * Add parameter hooks to inventory plugin iocage. * Add changelog fragment. * Update plugins/inventory/iocage.py Co-authored-by: Felix Fontein <felix@fontein.de> * Parameter renamed to hooks_results * Fix DOCUMENTATION YAML 4-space indentation. * Fix DOCUMENTATION YAML 2-space indentation. * Update changelogs/fragments/9651-iocage-inventory-hooks.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Add note about activated pool mountpoint. --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit fdd1331) Co-authored-by: Vladimir Botka <vbotka@gmail.com>

…lugin iocage (ansible-collections#9651) * Add parameter hooks to inventory plugin iocage. * Add changelog fragment. * Update plugins/inventory/iocage.py Co-authored-by: Felix Fontein <felix@fontein.de> * Parameter renamed to hooks_results * Fix DOCUMENTATION YAML 4-space indentation. * Fix DOCUMENTATION YAML 2-space indentation. * Update changelogs/fragments/9651-iocage-inventory-hooks.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Add note about activated pool mountpoint. --------- Co-authored-by: Felix Fontein <felix@fontein.de>

Add parameter hooks to inventory plugin iocage.

8f182b8

ansibullbot added feature This issue/PR relates to a feature request inventory inventory plugin plugins plugin (any type) labels Jan 30, 2025

Add changelog fragment.

1b43185

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Jan 30, 2025

felixfontein reviewed Jan 31, 2025

View reviewed changes

Update plugins/inventory/iocage.py

97197b0

Co-authored-by: Felix Fontein <felix@fontein.de>

Parameter renamed to hooks_results

318238b

russoz approved these changes Feb 3, 2025

View reviewed changes

Fix DOCUMENTATION YAML 4-space indentation.

a1d172c

ansibullbot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Feb 4, 2025

ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Feb 4, 2025

Fix DOCUMENTATION YAML 2-space indentation.

90301ae

felixfontein mentioned this pull request Feb 9, 2025

ansible-doc: fix indent and line wrapping for first line of (sub-)option and (sub-)return value descriptions ansible/ansible#84690

Merged

felixfontein reviewed Feb 10, 2025

View reviewed changes

Comment thread changelogs/fragments/9651-iocage-inventory-hooks.yml Outdated

Update changelogs/fragments/9651-iocage-inventory-hooks.yml

c8f8ed1

Co-authored-by: Felix Fontein <felix@fontein.de>

felixfontein approved these changes Feb 11, 2025

View reviewed changes

Add note about activated pool mountpoint.

cd27805

felixfontein merged commit fdd1331 into ansible-collections:main Feb 11, 2025

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 11, 2025

patchback Bot mentioned this pull request Feb 11, 2025

[PR #9651/fdd1331e backport][stable-10] Implement #9650 Add parameter hooks to inventory plugin iocage #9731

Merged

vbotka deleted the feature-iocage-hooks branch February 11, 2025 17:27

Akasurde mentioned this pull request Mar 17, 2025

Add parameter hooks to inventory plugin iocage #9650

Closed

1 task

Conversation

vbotka commented Jan 30, 2025

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

felixfontein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixfontein Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

vbotka Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfontein Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

vbotka Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

vbotka commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixfontein commented Feb 1, 2025

Uh oh!

vbotka commented Feb 1, 2025

Uh oh!

russoz left a comment

Choose a reason for hiding this comment

Uh oh!

vbotka commented Feb 4, 2025

Uh oh!

felixfontein commented Feb 8, 2025

Uh oh!

vbotka commented Feb 9, 2025

Uh oh!

felixfontein commented Feb 9, 2025

Uh oh!

Uh oh!

vbotka commented Feb 11, 2025

Uh oh!

patchback Bot commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to stable-10: 💚 backport PR created

Uh oh!

felixfontein commented Feb 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vbotka Feb 1, 2025 •

edited

Loading

vbotka commented Jan 31, 2025 •

edited

Loading

patchback Bot commented Feb 11, 2025 •

edited

Loading